Remove constant_function for AutoEnzyme
#74
Conversation
|
Let's wait for a review from Billy and Chris before merging (I added Billy as "read" contributor to the repo so that I can request reviews in the future, if that's ok) |
|
Let's just go straight to the |
|
I wanted to wait until the implementation exists on the Enzyme and DI side before reintroducing this kwarg. Our mistake last time was to introduce it before it actually did anything, and as a result we got the design wrong. |
|
But if @wsmoses is confident that |
|
I would probably do the yank now then design when ready. Being even more forward looking this should likely actually be a Union{Nothing, Enzyme.Annotation}, since the function could be nothing, constant, duplicated, or even active |
|
This constant function is not the appropriate name |
|
function_activity |
|
I mean that sounds like a good name to me, but it also might be wise to
wait til things are settled before choosing
…On Fri, Aug 2, 2024 at 11:42 AM Christopher Rackauckas < ***@***.***> wrote:
function_activity
—
Reply to this email directly, view it on GitHub
<#74 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJTUXABNMKRVTSUUD2VQGTZPOSEXAVCNFSM6AAAAABL333KG6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRVGY3TEMRRGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Okay then removing everything first is the right move. In this spirit, do you approve this PR? |
Warning
This PR and the associated release are breaking wrt 1.6.1, but we will yank 1.6.1 and 1.6.0 right after
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Following a long discussion with @ChrisRackauckas and @wsmoses, we decided to remove the
constant_functionsetting for now, and reinsert it later with different defaults (once Enzyme implements a few changes on their side). This PR does the removal and bumps ADTypes to v1.6.2, after which we will need to yank v1.6.0 and v1.6.1.